Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: enable or disable scatter dot on charts except scatter chart #769

Merged
merged 7 commits into from
Sep 11, 2020

Conversation

JennChao
Copy link
Contributor

Updates

Allow user to enable or disable scatter dot in charts except scatter chart.

Demo screenshot or recording

圖片

Review checklist (for reviewers only)

  • Demos all features
  • Documented/annotated
  • Matches UI/UX specs
  • Meets the code style guide
  • Accessible
  • Mobile first (responsive)
  • RTL support (bidirectional text)
  • Performant (limited bloat)

@JennChao JennChao requested review from natashadecoste, theiliad and a team as code owners August 27, 2020 03:37
@JennChao JennChao requested review from sophiiae and removed request for a team August 27, 2020 03:37

export class StackedScatter extends Scatter {
type = "scatter-stacked";

render(animate: boolean) {
const isScatterEnabled = Tools.getProperty(this.model.getOptions(), "scatterDotEnabled");
console.log("!!! this.configs.ignoreScatterDisabled: ", this.configs.ignoreScatterDisabled);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

console.logs?

@@ -146,6 +146,7 @@ const axisChart: AxisChartOptions = Tools.merge({}, chart, {
axes,
timeScale,
grid,
scatterDotEnabled: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably under the points flag as enabled?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we move scatterDotEnabled under the points flag as enabled, then only scatter chart can access the config option. However, axis charts, such as line chart and area chart, also need to access this config option as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think this'd be the case. Any chart that uses scatter should extend scatter chart and their options should do the same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

圖片

Sometimes we don't want to have the scatter dot displayed. That's the reason why I setup this scatterDotEnabled option for letting the user enable or disable the scatter dot. And please note that, there will always be scatter dot displayed on scatter charts. Only line chart, area chart, and step chart can have the option to disable scatter dot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes in the screenshot you have above, AreaChart should be extending scatter which means the enabled flag would be available in there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I need to figure out what you mean by saying extending scatter chart. Both area chart and scatter chart extend AxisChart and have scatter component in them. That's the reason why we can see the scatter dot. And from my understanding, scatter component will only be newed in AxisChart.
圖片

From what I have observed above, I think whether we want to show or not show the scatter dot, Scatter component, should not be configure in scatterChart option. scatterDotEnabled should be specify in axisChart option so all the axis charts with Scatter component can access this config option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what @theiliad is saying is that the ScatterChartOptions extends the AxisChartOptions meaning that all charts that have Scatter dots will have ScatterChartOptions in addition to the AxisChartOptions.

so if you change ScatterChartOptions like this (charts.ts):

/**
 * options specific to scatter charts
 */
export interface ScatterChartOptions extends AxisChartOptions {
	/**
	 * options for the points
	 */
	points?: {
		/**
		 * sets the radius of the point
		 */
		radius: number;
		fillOpacity?: number;
		filled?: boolean;
		enabled?: boolean;
	};
}

than you can update how you access the config in both scatter-stacked.ts and scatter.ts:
const isScatterEnabled = Tools.getProperty(this.model.getOptions(), "points", "enabled");

@@ -44,7 +44,9 @@ export class ScatterChart extends AxisChart {
new TwoDimensionalAxes(this.model, this.services),
new Grid(this.model, this.services),
new Ruler(this.model, this.services),
new Scatter(this.model, this.services),
new Scatter(this.model, this.services, {
ignoreScatterDisabled: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably need a more readable name here. If I was looking at this file for the first time I probably wouldn't understand what this flag would do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about alwaysEnableScatterDot?

@JennChao JennChao requested a review from theiliad August 28, 2020 09:16
@@ -146,6 +146,7 @@ const axisChart: AxisChartOptions = Tools.merge({}, chart, {
axes,
timeScale,
grid,
scatterDotEnabled: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think this'd be the case. Any chart that uses scatter should extend scatter chart and their options should do the same

@JennChao JennChao requested a review from theiliad September 1, 2020 10:15
@@ -146,6 +146,7 @@ const axisChart: AxisChartOptions = Tools.merge({}, chart, {
axes,
timeScale,
grid,
scatterDotEnabled: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes in the screenshot you have above, AreaChart should be extending scatter which means the enabled flag would be available in there

@JennChao JennChao requested a review from theiliad September 2, 2020 09:36
Copy link
Member

@theiliad theiliad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag should really be under points. If a user manually chooses to also disable scatter dots on just a scatter chart, that's just a user error. They will immediately realize that this isn't a great idea and enable the points again. No reason to cause inconsistencies to APIs here

image

@@ -146,6 +146,7 @@ const axisChart: AxisChartOptions = Tools.merge({}, chart, {
axes,
timeScale,
grid,
scatterDotEnabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what @theiliad is saying is that the ScatterChartOptions extends the AxisChartOptions meaning that all charts that have Scatter dots will have ScatterChartOptions in addition to the AxisChartOptions.

so if you change ScatterChartOptions like this (charts.ts):

/**
 * options specific to scatter charts
 */
export interface ScatterChartOptions extends AxisChartOptions {
	/**
	 * options for the points
	 */
	points?: {
		/**
		 * sets the radius of the point
		 */
		radius: number;
		fillOpacity?: number;
		filled?: boolean;
		enabled?: boolean;
	};
}

than you can update how you access the config in both scatter-stacked.ts and scatter.ts:
const isScatterEnabled = Tools.getProperty(this.model.getOptions(), "points", "enabled");

…to scatter-option

* 'master' of https://github.com/JennChao/carbon-charts:
  v0.36.4
  Merge pull request carbon-design-system#761 from JennChao/sparkline
  Merge pull request carbon-design-system#793 from hlyang397/update-initial-zoom-domain
  fix the defect of label tooltip not showing when using custom tooltip (carbon-design-system#787)
  v0.36.3
  Merge pull request carbon-design-system#785 from sophiiae/skeleton-with-data
@theiliad theiliad merged commit 3131942 into carbon-design-system:master Sep 11, 2020
JennChao added a commit to JennChao/carbon-charts that referenced this pull request Sep 14, 2020
…to linearGradient

* 'master' of https://github.com/JennChao/carbon-charts:
  v0.37.0
  Merge pull request carbon-design-system#780 from natashadecoste/fix-meter-max-val
  feat: enable or disable scatter dot on charts except scatter chart (carbon-design-system#769)
  feat: create options for tick rotation (carbon-design-system#770)
JennChao added a commit to JennChao/carbon-charts that referenced this pull request Sep 14, 2020
…to axis-option

* 'master' of https://github.com/JennChao/carbon-charts:
  v0.37.0
  Merge pull request carbon-design-system#780 from natashadecoste/fix-meter-max-val
  feat: enable or disable scatter dot on charts except scatter chart (carbon-design-system#769)
  feat: create options for tick rotation (carbon-design-system#770)

fix: enable zoom in functionality while axis is disabled
JennChao added a commit to JennChao/carbon-charts that referenced this pull request Sep 15, 2020
…to ruler-option

* 'master' of https://github.com/JennChao/carbon-charts:
  v0.37.1
  Merge pull request carbon-design-system#800 from metonym/fix-svelte-base-chart
  v0.37.0
  Merge pull request carbon-design-system#780 from natashadecoste/fix-meter-max-val
  feat: enable or disable scatter dot on charts except scatter chart (carbon-design-system#769)
  feat: create options for tick rotation (carbon-design-system#770)
JennChao added a commit to JennChao/carbon-charts that referenced this pull request Sep 15, 2020
…to tooltip-option

* 'master' of https://github.com/JennChao/carbon-charts:
  v0.37.1
  Merge pull request carbon-design-system#800 from metonym/fix-svelte-base-chart
  v0.37.0
  Merge pull request carbon-design-system#780 from natashadecoste/fix-meter-max-val
  feat: enable or disable scatter dot on charts except scatter chart (carbon-design-system#769)
  feat: create options for tick rotation (carbon-design-system#770)
  v0.36.4
  Merge pull request carbon-design-system#761 from JennChao/sparkline
  Merge pull request carbon-design-system#793 from hlyang397/update-initial-zoom-domain
  fix the defect of label tooltip not showing when using custom tooltip (carbon-design-system#787)
  v0.36.3
  Merge pull request carbon-design-system#785 from sophiiae/skeleton-with-data
theiliad pushed a commit to theiliad/carbon-charts that referenced this pull request May 17, 2021
…arbon-design-system#769)

* feat: enable or disable scatter dot on charts except scatter chart

* refactor: rename variable

* fix: always enable scatter dot for bubble chart

* refactor: have enabled option within points
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants